Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate $BB in YieldDistributor #94

Merged
merged 12 commits into from
Sep 22, 2024
Merged

Integrate $BB in YieldDistributor #94

merged 12 commits into from
Sep 22, 2024

Conversation

RonTuretzky
Copy link
Collaborator

#77

@RonTuretzky RonTuretzky requested review from daopunk and bagelface and removed request for daopunk September 18, 2024 09:44
@RonTuretzky
Copy link
Collaborator Author

Probably should make a new deploy script for "from scratch" deploys, however for practical purposes having these deploy scripts separated is useful.

I elected to not replicate the tests for buttered bread as it is also an ERC20Votes and all that functionality is already tested for $BREAD

Copy link
Collaborator

@daopunk daopunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than potential non-set revert issue

src/YieldDistributor.sol Show resolved Hide resolved
src/YieldDistributor.sol Outdated Show resolved Hide resolved
@RonTuretzky
Copy link
Collaborator Author

Will be adding a test soon

src/YieldDistributor.sol Outdated Show resolved Hide resolved
@@ -66,8 +69,15 @@ contract YieldDistributor is IYieldDistributor, OwnableUpgradeable {
address[] memory _projects
) public initializer {
__Ownable_init(msg.sender);

if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more readable personally

if (
    _bread == address(0) ||
    _butteredBread == address(0) ||
    _precision == 0 ||
    _minRequiredVotingPower == 0 ||
    _maxPoints == 0 ||
    _cycleLength == 0 ||
    _yieldFixedSplitDivisor == 0 ||
    _lastClaimedBlockNumber == 0 ||
    _projects.length == 0
) {
    revert MustBeGreaterThanZero();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatter overrides this.. do you know if there's a comment I can add for it to ignore this?

Copy link
Contributor

@bagelface bagelface Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from changing line_length, which i wouldn't recommend, not really. all good

src/YieldDistributor.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@bagelface bagelface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments

Copy link
Contributor

@bagelface bagelface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

uint256 _lastClaimedBlockNumber = stdJson.readUint(config_data, "._lastClaimedBlockNumber");
uint256 _yieldFixedSplitDivisor = stdJson.readUint(config_data, "._yieldFixedSplitDivisor");
// See test/YieldDistributor.t.sol for explanation of these values
uint256 _minRequiredVotingPower = stdJson.readUint(config_data, "._minRequiredVotingPower");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i recommend in the future, inheriting the deployment script from the Scripts folder into the test setup, which will also inherit the stdJson datas. but for now, this works

@RonTuretzky
Copy link
Collaborator Author

Opened #97 to address issues introduced with this PR , merging for now :)

@RonTuretzky RonTuretzky merged commit 1989246 into dev Sep 22, 2024
@bagelface bagelface deleted the integrate_bb_in_yd branch September 22, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants